Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(docs): Updated examples topic to use ECS L2 construct #1195

Merged
merged 17 commits into from
Dec 7, 2018

Conversation

Doug-AWS
Copy link
Contributor

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should ultimately also add a load balancer to make it do something useful I think.


Creating an App with Multiple Stacks
====================================
Creating an |ECS| Construct
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't call it "creating an ECS construct", that implies you're building a reusable thing.

Maybe "Writing an |ECS| CDK app" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

Implement the class **MyStack** in the *my-stack* sub-folder,
that extends the |stack-class| class
(this is the same code as shown in the :doc:`concepts` topic).
* Automatic security group opening for load balancers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not sentences, maybe rephrase to a list of the form "it will open up ..." etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. I wrote these based on some notes and forgot to re-visit.


import { Stack, StackProps } from '@aws-cdk/cdk'
Step 1: Create the Directory and Initialze the |cdk|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo. Wouldn't capitalize Directory and Initialize here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is AWS docs style. We capitalize pretty much everything but articles (the, and, etc.).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still typo

import ec2 = require('@aws-cdk/aws-ec2');
import rds = require('@aws-cdk/aws-rds');
import cdk = require('@aws-cdk/cdk');
There are two different ways of creating a serverless infrastructure with |ECS|:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would write "two different ways of running your container tasks"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

const vpc = new ec2.VpcNetwork(this, 'VPC');
This example creates a Fargate service,
which requires a VPC, a cluster, a task definition, and a security group.
Later on we'll show you how to launch an EC2 service.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how to launch services on EC2 instances you manage yourself

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

}
}
// The task definition must have at least one container.
// Otherwise you cannot synthesize or deploy your app.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like a constraint if you put it like this. But also, if you don't add a container, WHAT are you even launching? Empty tasks that do nothing? That's not very useful.

So I wouldn't call attention to it in this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rewrote the first comment line and removed the second comment line.

// The task definition must have at least one container.
// Otherwise you cannot synthesize or deploy your app.
taskDefinition.addContainer('MyContainer', {
image: ecs.ContainerImage.fromDockerHub('MyDockerImage') // Required
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe launch amazon/amazon-ecs-sample, that's a container that at least does something.

taskDefinition: taskDefinition, // Required
cluster: cluster, // Required
desiredCount: 6, // Default is 1
securityGroup: securityGroup, // Required
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep


const app = new cdk.App();
const securityGroup = new ec2.SecurityGroup(this, 'MySecurityGroup', {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

cluster: cluster, // Required
desiredCount: 6, // Default is 1
securityGroup: securityGroup, // Required
platformVersion: ecs.FargatePlatformVersion.Latest // Default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't even call attention to this, just leave it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@Doug-AWS
Copy link
Contributor Author

Doug-AWS commented Nov 20, 2018

I've updated the list of benefits and am deploying the code now to make sure it works (50 minutes and counting):

28/30 | 11:50:59 | CREATE_COMPLETE | AWS::EC2::Route | MyVpc/PrivateSubnet2/DefaultRoute (MyVpcPrivateSubnet2DefaultRoute9CE96294)
28/30 Currently in progress: MyFargateService8825BC17

@Doug-AWS
Copy link
Contributor Author

Strange. I nuked my deployment and the CFN stack. then started all over with a fresh project. The code deployed in about 3 1/2 minutes. The template grew from about 300 lines to almost 500.


import { Stack, StackProps } from '@aws-cdk/cdk'
Step 1: Create the Directory and Initialze the |cdk|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still typo


.. _dynamodb_example:
Update *my_ecs_construct.ts* in the *bin* directory to only contain the following code:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will have gotten changed to lib/my_ecs_construct_stack.ts by the time this goes out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the typo (sheesh); I just tested cdk init --language typescript in 0.18.1 and it still creates the TS file in the bin folder.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know. The change is not released yet. That's why I wrote:

by the time this goes out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this part as the template has changed.

.. code-block:: ts

// Add capacity to it
cluster.addDefaultAutoScalingGroupCapacity({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, no. This is to add EC2 capacity to run Ec2Services on. You've been creating a FargateService so you don't need to do this.

You want what this section says:

https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-ecs/README.md#task-autoscaling

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take it out for now.


.. _dynamodb_example:
Update *my_ecs_construct.ts* in the *bin* directory to only contain the following code:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know. The change is not released yet. That's why I wrote:

by the time this goes out.

readCapacity: 5,
writeCapacity: 5
});
const app = new cdk.App();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact these lines will also be gone.

These lines will live in a file in the bin directory, the other lines will live in a file in the lib directory.

Here's the new version of the app template: https://github.com/awslabs/aws-cdk/tree/master/packages/aws-cdk/lib/init-templates/app/typescript

|CFN| displays information about the dozens of steps that
it takes as it deploys your app.

That's how easy it is to create a Fargate service to run a Docker image.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's true. The example is not yet useful without a load balancer though, since this is a web service but you will be unable to hit the endpoints (because they'll be in a private VPC if I'm not mistaken).

Just sayin'.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you planning to make this section longer, or is it the plan to publish like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a load balancer and auto-scaling for tasks to the example. Once the new template is released, I'll fix the references:

my_ecs_construct.ts in the bin directory -> my_ecs_stack.ts in the lib directory

@Doug-AWS Doug-AWS changed the title feat(docs): Updated examples topic to use ECS L2 construct (first cut) feat(docs): Updated examples topic to use ECS L2 construct Dec 3, 2018
});

// Add capacity to it
cluster.addDefaultAutoScalingGroupCapacity({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need capacity if you're starting a Fargate task

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I saw your comment to Paul in issue #1279.

@Doug-AWS
Copy link
Contributor Author

Doug-AWS commented Dec 6, 2018

I'm still blocked from confirming cdk deploy by issue #1279

@@ -65,7 +67,7 @@ gives you a leg up on using AWS services by providing the following benefits:
* Automatically adds permissions for |ECR| if you use an image from |ECR|
When you use an image from |ECR|, the |cdk| adds the correct permissions.

* Convenient API for autoscaling
* Automatic autoscaling
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean? It's not automatic yet. I think we may have miscommunicated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so what should it say? Should I go back to:

  • Convenient API for autoscaling
    The |cdk| supplies a method so you can autoscale instances when you use an |EC2| cluster;
    this functionality is done automatically when you use an instance in a Fargate cluster.

@rix0rrr rix0rrr merged commit 9a7709e into master Dec 7, 2018
@rix0rrr rix0rrr deleted the dougsch/replace-examples branch December 7, 2018 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants